Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a GitHub action for periodically running vale and storing the results #281

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

k-dimple
Copy link
Contributor

@k-dimple k-dimple commented Sep 12, 2024

Current automated documentation checks do not include style checks. To include style checks using vale, this PR adds a GitHub action that runs once a week and stores the results in a GitHub artifact that can be downloaded.

Addresses DOCPR-876

Copy link
Contributor

@SecondSkoll SecondSkoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of things to think about:

This will change fairly significantly as we re-assess severity levels.

How useful is running periodic checks in comparison to running on PR actions? We could just implement checks as non-blocking, as we will eventually want to turn that switch on, and it would save us some work further down the line.

I don't think this will provide useful data until Vale is in a more stable state - and we'll continue to iterate next cycle to refine Vale's output.

.github/workflows/periodic-style-checks.yml Outdated Show resolved Hide resolved
@k-dimple
Copy link
Contributor Author

k-dimple commented Sep 13, 2024

@SecondSkoll Severity levels? (DOCPR-776 seems to be the wrong one)

@evilnick was there a specific reason for wanting periodic vale checks instead of running them as non-blocking PR actions?

Yes, we could wait for vale to become more stable before including it in our workflows.

@SecondSkoll
Copy link
Contributor

SecondSkoll commented Sep 13, 2024

@SecondSkoll Severity levels? (DOCPR-776 seems to be the wrong one)

@evilnick was there a specific reason for wanting periodic vale checks instead of running them as non-blocking PR actions?

Yes, we could wait for vale to become more stable before including it in our workflows.

Quite right, wrong issue. Yes, changing the severity levels will change the output, also any refinements we make after testing. Presumably next cycle will involve some level of optimisation and reworking of rules - otherwise Vale is going to cause a lot of noise.

@evilnick
Copy link
Contributor

evilnick commented Sep 20, 2024

I am going to be tweaking the default Vale config this pulse so we can push a v1.0 out.
For the purposes of this task though, I think we should pursue periodic executions. The data may be junk for now, but it will be there.
The reason for periodic vs PR based run is that, for the metrics and tests we will eventually want snapshots of the existing docs as published.
The PR-based rules are so you know what you are adding, the periodics are so we assess state of current published docs. As I say, the data may be junk to begin with, but hopefully that will improve (and we will be able to see that too).

So for example, maybe on a PR the pa11y text complains about something. You may chose to see it as junk, or merge anyhow if it is non blocking. Those tests don't tell us anything about the docs that are published. a recurring task which logs a dozen a11y failures, or broken links is more useful

uses: actions/upload-artifact@v4
with:
name: vale-results
path: vale_results_clean.txt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest we put these in the /metrics directory the other testing PR adds

Copy link
Contributor

@evilnick evilnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍰 but see comment

@k-dimple k-dimple mentioned this pull request Sep 24, 2024
@SecondSkoll
Copy link
Contributor

I'm going to close this due to our discussion from 2024/09/26.
Feel free to reopen this if necessary.

@evilnick evilnick reopened this Oct 3, 2024
@evilnick
Copy link
Contributor

evilnick commented Oct 3, 2024

can we modify this action so it just runs and doesn't store output ( i mean, it can output to the console and we can than see it in the run logs anyhow).

@SecondSkoll
Copy link
Contributor

All changes from main have been merged in to the use-canonical-sphinx-extension branch. It would be great to submit a PR against that branch instead as it's about to become the main branch.

@k-dimple
Copy link
Contributor Author

k-dimple commented Oct 3, 2024

Sure, I can submit a PR for it in that branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants